feat(core,isthmus): add DynamicParameter expression support#752
Open
bvolpato wants to merge 1 commit intosubstrait-io:mainfrom
Open
feat(core,isthmus): add DynamicParameter expression support#752bvolpato wants to merge 1 commit intosubstrait-io:mainfrom
bvolpato wants to merge 1 commit intosubstrait-io:mainfrom
Conversation
This was referenced Mar 16, 2026
test: add DynamicParameter and DynamicParameterBinding roundtrip tests
substrait-io/substrait-rs#481
Closed
nielspardon
reviewed
Mar 18, 2026
Member
There was a problem hiding this comment.
These testcases feel a bit inconsistent. Sometimes they only assert the type, sometimes they only assert the parameter reference. Wouldn't it make sense to always check both?
Member
|
Do you plan to also work on implementing the |
bvolpato
added a commit
to substrait-io/substrait-go
that referenced
this pull request
Mar 22, 2026
## Summary Add full support for `DynamicParameter` expressions and plan-level `DynamicParameterBinding`, enabling parameterized queries in substrait-go. This mirrors the functionality recently added in [substrait-java#752](substrait-io/substrait-java#752), ensuring Go consumers can create and read plans containing dynamic parameters. ## Changes ### Expression (`expr/expression.go`) - New `DynamicParameter` struct implementing the full `Expression` interface (`String`, `ToProto`, `Equals`, `Visit`, `IsScalar`, `GetType`, `ToProtoFuncArg`) - Proto deserialization in `ExprFromProto()` with nil safety check ### Builder (`expr/builder.go`) - `ExprBuilder.DynamicParam(outputType, paramRef)` method with `dynamicParamBuilder` - Implements `BuildExpr()` and `BuildFuncArg()` for use as function arguments ### Plan bindings (`plan/common.go`, `plan/plan.go`, `plan/builders.go`) - `DynamicParameterBinding` struct mapping parameter anchors to literal values - `Plan.ParameterBindings()` accessor - `FromProto()` / `ToProto()` serialization for parameter bindings - `Builder.PlanWithBindings()` method on the `Builder` interface ## Testing 17 new tests across two test files: - `expr/dynamic_parameter_test.go` — 11 tests covering basic construction, nullability, equality, visit, proto roundtrip (5 types), nil proto error, builder (3 cases), builder as function argument, and multiple parameters - `plan/dynamic_parameter_test.go` — 6 tests covering filter plan with bindings, project plan, multiple bindings, no bindings, JSON parsing roundtrip, and end-to-end builder usage All existing tests continue to pass with zero regressions.
bvolpato-dd
force-pushed
the
bv/dynamic-parameter-support
branch
from
2bf9bd6 to
a1807e4
Compare
Implement full support for Substrait DynamicParameter expressions, enabling parameterized placeholders in plan bodies instead of embedded literals. This maps bidirectionally to Calcite's RexDynamicParam (JDBC ? bind parameters). Changes: - Add Expression.DynamicParameter POJO with type and parameterReference - Wire visitor pattern across all expression visitors - Add proto conversion (POJO<->Proto) in both directions - Add Calcite conversion (RexDynamicParam<->DynamicParameter) - Replace UnsupportedOperationException in visitDynamicParam - Add debug stringification in ExpressionStringify - Add 20 tests covering proto roundtrips, Calcite conversions, and full end-to-end roundtrips
bvolpato-dd
force-pushed
the
bv/dynamic-parameter-support
branch
from
a1807e4 to
96bcc42
Compare
Member
Author
|
Thanks @nielspardon! Great question. I've rebased this PR on the latest Regarding I think it makes sense to handle
I'll open a follow-up issue to track |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add full support for Substrait
DynamicParameterexpressions, enabling parameterized placeholders (analogous to JDBC?bind parameters) in plan bodies instead of embedded literals. This maps bidirectionally to Calcite'sRexDynamicParam.DynamicParameter enables plan reuse — the same plan structure can be compiled and cached once, then executed with different parameter values without re-planning. This is essential for prepared statement workflows and for exchanging plans between engines without leaking literal values.
Changes
Core POJO + Proto layer:
Expression.DynamicParameter— immutable POJO withtypeandparameterReferencefieldsExpressionVisitor/AbstractExpressionVisitor— newvisit(DynamicParameter)methodExpressionProtoConverter— POJO→Proto conversionProtoExpressionConverter— Proto→POJO conversion (DYNAMIC_PARAMETERcase)ExpressionCopyOnWriteVisitor— leaf-node handling (returnsOptional.empty())Calcite integration (isthmus):
RexExpressionConverter.visitDynamicParam()— replacesUnsupportedOperationExceptionwith actual Calcite→Substrait conversionExpressionRexConverter.visit(DynamicParameter)— Substrait→Calcite conversionDebug support:
ExpressionStringify— added DynamicParameter stringificationTesting
20 tests total, all passing:
DynamicParameterRoundtripTestin core) — POJO↔Proto↔POJO for I64, nullable STRING, FP64, I32, DATE, BOOLEAN, DECIMAL, TIMESTAMPDynamicParameterTest) — direct Calcite↔Substrait conversions and project roundtripsDynamicParameterRoundtripTestin isthmus) — filter/projection/multi-type POJO roundtrips, Calcite-originated RexDynamicParam roundtrips, and full Substrait→Calcite→Substrait bidirectional roundtripsAll existing tests in
coreandisthmuscontinue to pass with zero regressions. PMD and Spotless checks are clean.